Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement LoopModeOverride to AnimationNodeAnimation to properly manage the loop state for each AnimationTree #79400

Closed
wants to merge 1 commit into from

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Jul 12, 2023

Closes #79208.

Implement the LoopModeOverride parameter to the AnimationNodeAnimation.

Context: #79208 (comment)

AutoAdvance should not be executed for looping animations, since it is undecided where the animation will end. However, there are cases where AutoAdvance is executed at the appropriate time after looping several times.

The straightforward way to solve this is to disable the Animation's Loop at any intended timing. However, changing the Loop property of an Animation resource will affect all the multiple AnimationTrees and AnimationPlayers that reference it.

Therefore, by implementing LoopModeOverride as a parameter of AnimationNodeAnimation, the loop state of the animation can be managed for each AnimationTree as a node.

This makes it easy to manage the looping state of animations for multiple characters.

image

@TokageItLab TokageItLab force-pushed the loop-override branch 3 times, most recently from 1c4e065 to 48d7883 Compare July 21, 2023 19:24
Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I see why merging the enum made the switch statement messy.

Comment on lines +3126 to +3127
default: {
} break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so the challenge with combining LoopMode enum into Animation and AnimationNodeAnimation is that the extra enum value may be possible to set in loop_mode here, and the unused case statement may be needed.

Should the default case be the same as LOOP_NONE?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the default case be the same as LOOP_NONE?

It will break compatibility. Also, it should not be overridden by default. So No Override should be the default for AnimationNodeAnimation.

@TokageItLab TokageItLab requested a review from lyuma August 20, 2023 13:26
@TokageItLab
Copy link
Member Author

@lyuma How should I do?

@SaracenOne
Copy link
Member

I'm a little bit nervous about this workaround for a few reasons. While I'm not against introducing an AnimationTree specific overwrite of looping state for animation, I feel that implementing it in the parameter list feels quite janky. I'm also unsure if this solves the underlying issues with the AtEnd transition. While there may be some ambiguity on what constitution an AtEnd transition, I would like to investigate a more explicit fix. I could still be in favour of looping overrides in a more general sense, but I am unsure of this specific implementation.

@TokageItLab
Copy link
Member Author

TokageItLab commented Sep 16, 2023

The cause of this problem is clear.

The progress of a StateMachine (and the same for the all chained downstream node) is taken from the remaining time of its upstream animation nodes. However, upstream nodes can be nested, and in that case it is not clear which node's remaining time should be prioritized.

Currently, the node with the longest remaining time is selected, but this may cause unintended transitions (e.g., you want to fire AtEnd in favor of End of an animation with a short remaining time; Imagine multiple nested loop animations of different lengths without sync).

Therefore, currently, for such cases where the remaining time is ambiguous, we have no choice but to set the time to infinite, and the only way to transition is to manually break the loop. If it force strange transitions, we cannot provide workarounds, but if it stays in a state of no transitions, we can provide some workarounds, such as using method tracks or interrupting loops.

Even in such a case, the StateMachine does not know the progress at the time of the transition operated, so it must choose between a forced transition or an interruption of the nested upstream animation loop.

@SaracenOne
Copy link
Member

Okay, I'm starting to understand the issue a bit better now. I would be curious if you felt that there could be some value in adding warnings to AnimationTree inspector in order to communicate some of this to the end user?

@TokageItLab
Copy link
Member Author

TokageItLab commented Sep 16, 2023

Instead of a progress bar, it might be better to have a notation telling people that the length is infinite. Like ♾️ or [infinity]?

@TokageItLab
Copy link
Member Author

Superseded by #87171

@TokageItLab TokageItLab removed this from the 4.3 milestone Jan 14, 2024
@TokageItLab TokageItLab deleted the loop-override branch February 14, 2024 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Animation travel not working when an animation is looped
4 participants